RSDK-14030 - update to cpp-sdk v37#16
Conversation
…nsors to construct a jacobian the git diff for that work will be smaller
|
I will also have to update the yaskawa driver to be on the newer versions of the cpp-sdk so that we can actually construct a model table, but that is another body of work |
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
Looks pretty good, a few small things to address.
We are going to need to do the same work in UR before we can upgrade UR to a version of trajex that contains this change, since otherwise the FetchContent stuff is going to end up pulling a newer version of the C++ SDK transitively through trajex than UR is prepared to handle. Do we have a ticket to do that work?
| mlmodel::mlmodel(vsdk::Dependencies deps, vsdk::ResourceConfig config) : MLModelService(config.name()) { | ||
| reconfigure(deps, config); | ||
| } | ||
| mlmodel::mlmodel(vsdk::Dependencies /*deps*/, vsdk::ResourceConfig config) : MLModelService(config.name()), config_(parse_config(config)) {} |
There was a problem hiding this comment.
You don't need the commented out parameter name, anon is fine.
|
|
||
| mutable std::shared_mutex config_mutex_; | ||
| config config_; | ||
| static config parse_config(const ::viam::sdk::ResourceConfig& cfg); |
There was a problem hiding this comment.
This can live as a static function on config itself. Just call it create or from_resource_config or similar.
Or if you want/need to keep it on class mlmodel for some reason, it should have a trailing _ since it is a private method.
| set(VIAM_TRAJEX_EIGEN3_VERSION_MINIMUM "") # TODO: Decide this | ||
| set(VIAM_TRAJEX_JSONCPP_VERSION_MINIMUM "") # TODO: Decide this | ||
| set(VIAM_TRAJEX_VIAMCPPSDK_VERSION_MINIMUM 0.31.0) | ||
| set(VIAM_TRAJEX_VIAMCPPSDK_VERSION_MINIMUM 0.37.0) |
There was a problem hiding this comment.
Have a careful look for where else this is specified. I think at least the conan file. Dockerfile?
There was a problem hiding this comment.
i've updated the conan file, but there is no Dockerfile in this repo.
|
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
LGTM. You are right about the docker file, that only exists in UR not in this repo. When you do the UR ticket you will need to update the docker file, IIRC.
2 participants
all this so that when we start using the work here: viamrobotics/viam-cpp-sdk#645
the next git diff is smaller.